Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLOC-4489] Use detailed volume list and update to a version of mimic that supports that endpoint #2892

Merged
merged 5 commits into from
Sep 8, 2016

Conversation

wallrj
Copy link
Contributor

@wallrj wallrj commented Aug 30, 2016

Fixes: https://clusterhq.atlassian.net/browse/FLOC-4489
Depends on: ClusterHQ/mimic#1

I re-instated the use of detailed volume list.
This ensures that volume attachment information is returned, even when interacting with Cinder v2 API.

One of our tests attempts to hit the volumes/detail endpoint with a mimic based fake REST server.
So I also had to fork andupdate mimic to support that endpoint.

I'll post devstack based test results shortly.

@wallrj
Copy link
Contributor Author

wallrj commented Aug 30, 2016

I'm trying to understand some separate failures on devstack


Traceback (most recent call last):
  File "/home/ubuntu/flocker/flocker/node/agents/functional/test_cinder.py", line 444, in test_get_device_path_no_attached_disks
    device_path = self.blockdevice_api.get_device_path(volume.id)
  File "/home/ubuntu/flocker/flocker/node/agents/cinder.py", line 726, in get_device_path
    device_path = self._get_device_path_virtio_blk(cinder_volume)
  File "/home/ubuntu/flocker/flocker/node/agents/cinder.py", line 678, in _get_device_path_virtio_blk
    raise UnattachedVolume(volume.id)
flocker.node.agents.blockdevice.UnattachedVolume: b0c589bd-20b6-429d-9686-936352775c1b


flocker.node.agents.functional.test_cinder.CinderAttachmentTests.test_get_device_path_no_attached_disks
-------------------------------------------------------------------------------
Ran 1 tests in 15.566s

@wallrj
Copy link
Contributor Author

wallrj commented Sep 2, 2016

I'm trying to understand some separate failures on devstack

These seem to occur only on Ubuntu 14.04 and the problem is that the hotplug system doesn't always create a /dev/disks/by-id symlink for the device that's been attached.

I can force the symlink by running udevadm trigger

The tests are much more reliable on Ubuntu 16.04

@wallrj
Copy link
Contributor Author

wallrj commented Sep 2, 2016

(4489) ubuntu@richardw-xenial-machine-1:~/flocker$ sudo /usr/bin/env FLOCKER_FUNCTIONAL_TEST_CLOUD_CONFIG_SECTION=storage-drivers.devstack FLOCKER_FUNCTIONAL_TEST_CLOUD_CONFIG_FILE=/home/ubuntu/acceptance.yml FLOCKER_FUNCTIONAL_TEST=TRUE $(type -p trial) flocker.node.agents.functional.test_cinder
sudo: unable to resolve host richardw-xenial-machine-1
flocker.node.agents.functional.test_cinder
  BlockDeviceAPIDestroyTests
    test_destroy_timesout ...                                              [OK]
  CinderAttachmentTests
    test_get_device_path_no_attached_disks ...                             [OK]
  CinderBlockDeviceAPIInterfaceTests
    test_attach_attached_volume ...                                        [OK]
    test_attach_destroyed_volume ...                                       [OK]
    test_attach_elsewhere_attached_volume ...                              [OK]
    test_attach_unattached_volume ...                                      [OK]
    test_attach_unknown_volume ...                                         [OK]
    test_attached_volume_listed ...                                        [OK]
    test_compute_instance_id_nonempty ...                                  [OK]
    test_compute_instance_id_unicode ...                                   [OK]
    test_created_is_listed ...                                             [OK]
    test_created_volume_attributes ...                                     [OK]
    test_destroy_destroyed_volume ...                                      [OK]
    test_destroy_unknown_volume ...                                        [OK]
    test_destroy_volume ...                                                [OK]
    test_detach_detached_volume ...                                        [OK]
    test_detach_unknown_volume ...                                         [OK]
    test_detach_volume ...                                                 [OK]
    test_device_size ...                                                   [OK]
    test_foreign_cluster_volume ...                                        [OK]
    test_foreign_volume ...                                                [OK]
    test_get_device_path_device ...                                        [OK]
    test_get_device_path_device_repeatable_results ...                     [OK]
    test_get_device_path_unattached_volume ...                             [OK]
    test_get_device_path_unknown_volume ...                                [OK]
    test_interface ...                                                     [OK]
    test_list_attached_and_unattached ...                                  [OK]
    test_list_volume_empty ...                                             [OK]
    test_listed_volume_attributes ...                                      [OK]
    test_multiple_volumes_attached_to_host ...                             [OK]
    test_name ...                                                          [OK]
    test_reattach_detached_volume ...                                      [OK]
  CinderCloudAPIInterfaceTests
    test_current_machine_is_live ...                                       [OK]
    test_interface ...                                                     [OK]
    test_list_live_nodes ...                                               [OK]
  CinderFromConfigurationTests
    test_no_immediate_authentication ...                                   [OK]
    test_no_retry_authentication ...                                       [OK]
  CinderHttpsTests
    test_verify_ca_path_no_match_fails ...                            [SKIPPED]
    test_verify_false ...                                             [SKIPPED]
  LazyLoadingProxyTests
    test_loader_called_once ...                                            [OK]
    test_loader_exceptions_raised ...                                      [OK]
  VirtIOCinderAttachmentTests
    test_disk_attachment_fails_with_conflicting_disk ...              [SKIPPED]
    test_get_device_path_correct_with_attached_disk ...               [SKIPPED]
    test_get_device_path_virtio_blk_error_without_udev ...            [SKIPPED]
    test_get_device_path_virtio_blk_symlink ...                       [SKIPPED]

===============================================================================
[SKIPPED]
Tests require a TLS auth_url endpoint beginning with https://. Found auth_url: http://172.99.85.118:5000/v2.0

flocker.node.agents.functional.test_cinder.CinderHttpsTests.test_verify_ca_path_no_match_fails
flocker.node.agents.functional.test_cinder.CinderHttpsTests.test_verify_false
===============================================================================
[SKIPPED]
Tests require the ``virsh`` command.

flocker.node.agents.functional.test_cinder.VirtIOCinderAttachmentTests.test_disk_attachment_fails_with_conflicting_disk
flocker.node.agents.functional.test_cinder.VirtIOCinderAttachmentTests.test_get_device_path_correct_with_attached_disk
flocker.node.agents.functional.test_cinder.VirtIOCinderAttachmentTests.test_get_device_path_virtio_blk_error_without_udev
flocker.node.agents.functional.test_cinder.VirtIOCinderAttachmentTests.test_get_device_path_virtio_blk_symlink
-------------------------------------------------------------------------------
Ran 45 tests in 401.702s

PASSED (skips=6, successes=39)

@@ -530,7 +530,7 @@ def list_volumes(self):
http://docs.rackspace.com/cbs/api/v1.0/cbs-devguide/content/GET_getVolumesDetail_v1__tenant_id__volumes_detail_volumes.html
"""
flocker_volumes = []
for cinder_volume in self.cinder_volume_manager.list(detailed=False):
for cinder_volume in self.cinder_volume_manager.list(detailed=True):
Copy link
Contributor

@wallnerryan wallnerryan Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a case where we do not always need detailed=True? I seem to remember the discussion that Cinder v1 would pass with detailed=False, thus the detailed information is not needed? maybe im making this up, but making sure we're not requested uneeded information that may slow down the process being that every volume may have more information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's right.

That's what led to this bug. I tested against Rackspace and saw the tests pass.
On OpenStack the detailed list doesn't include the attachment info.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be the case where we use False for v1 instead of always True for detailed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be possible but probably more trouble than it's worth.
At this point the cinder version detection has already taken place and the CinderBlockDeviceAPI has an object conforming to an interface that we've defined ICinderVolumeManager.
Since we're aiming to drop support for CinderV1 (Rackspace) I'm going to leave it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I wonder if we should drop v1 though, there may be private deployments that may want to use v1, but not sure.

@wallnerryan
Copy link
Contributor

wallnerryan commented Sep 6, 2016

Did a preliminary review, but will revisit when ready and change status in JIRA, i think because this was at the top of a stack of other changes. Also looks like 1 check failing, looks like the same failure happening on #2893

@wallnerryan
Copy link
Contributor

wallnerryan commented Sep 7, 2016

@wallrj
You mentioned...

I can force the symlink by running udevadm trigger

to get /dev/disks/by-id to show up. Is this something that needs to be fixed for 14.04, this sounds like a bug to me.

Also, stilling seeing the following error in the trusty cinder CI test

Traceback (most recent call last):
  File "/tmp/14124/local/lib/python2.7/site-packages/testtools/twistedsupport/_runtest.py", line 386, in _log_user_exception
    raise e
testtools.twistedsupport._spinner.TimeoutError: <flocker.acceptance.endtoend.test_diagnostics.DiagnosticsTests.test_export id=0x7f4c4d752b90> took longer than 300.0 seconds

]
test: flocker.acceptance.endtoend.test_dockerplugin.DockerPluginTests.test_create_container_with_v2_plugin_api
Build timed out (after 90 minutes). Marking the build as failed.
Build was aborted
Archiving artifacts
Archiving artifacts

If these concerns can safely be ignored or are unrelated then the PR looks good. Please answer the above but otherwise LGTM

@wallrj
Copy link
Contributor Author

wallrj commented Sep 8, 2016

Thanks @wallnerryan

to get /dev/disks/by-id to show up. Is this something that needs to be fixed for 14.04, this sounds like a bug to me.

Yeah, we do need to investigate that.

It only affects virtio OpenStack environments...which is where we do disk serial number based device path detection.

It shouldn't affect Rackspace, which uses Xen volume driver.

So I'll merge this and create followup issues:

@wallrj wallrj merged commit 726abc3 into master Sep 8, 2016
@wallrj wallrj deleted the cinder-volume-detail-FLOC-4489 branch September 8, 2016 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants